Add Bento Board slice and preview deploy workflow#3
Conversation
Co-authored-by: multica-agent <github@multica.ai>
|
Bento Board preview deployed. URL: https://NightProgrammers.github.io/games/previews/bento-board/pr-3/ If this is the first deployment, enable GitHub Pages once in repository settings and point it at the |
📝 WalkthroughWalkthroughIntroduces Bento Board, a complete new puzzle game application with data-driven mechanics, engine simulation, daily generation, a client-side UI with localStorage persistence, CSS styling, and automated GitHub Actions deployment of preview and stable builds to gh-pages. ChangesBento Board Game Application
Deployment Automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
bento-board/src/data.js (1)
80-165: ⚡ Quick winExtract duplicate stack-normalization helper.
createLevel(Lines 95-108) andcreateDailyTemplate(Lines 139-152) contain identical logic that walks each stack's items, extractskind === "layer"entries into a separatelayersarray (annotated withstackId), and replaces them with lightweight{ kind: "layer", id }references. Pulling it into a shared helper keeps the two builders in sync if the layer/item shape ever evolves.♻️ Proposed extraction
+function normalizeStacks(stackDefinitions) { + const layers = []; + const stacks = stackDefinitions.map((stackDefinition) => ({ + ...stackDefinition, + items: stackDefinition.items.map((item) => { + if (item.kind === "layer") { + layers.push({ ...item, stackId: stackDefinition.id }); + return { kind: "layer", id: item.id }; + } + return item; + }) + })); + return { stacks, layers }; +} + function createLevel({ ... }) { - const layers = []; - const normalizedStacks = stacks.map((stackDefinition) => ({ - ...stackDefinition, - items: stackDefinition.items.map((item) => { - if (item.kind === "layer") { - layers.push({ - ...item, - stackId: stackDefinition.id - }); - return { kind: "layer", id: item.id }; - } - return item; - }) - })); + const { stacks: normalizedStacks, layers } = normalizeStacks(stacks); return { ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/data.js` around lines 80 - 165, Extract the duplicated stack-normalization logic into a single helper (e.g., normalizeStacksAndExtractLayers or extractLayersFromStacks) that takes the stacks array and returns { stacks: normalizedStacks, layers } where each extracted layer is annotated with stackId and each original layer item is replaced with { kind: "layer", id }. Replace the inlined mapping in createLevel and createDailyTemplate to call this helper and assign returned stacks to the builders' stacks field and returned layers to their layers variable, preserving all other fields and the default tutorialSteps behavior in createLevel.bento-board/tests/engine.test.js (2)
67-162: 💤 Low valueConsider extracting fixture builders.
Both the overflow and jam test fixtures hand-roll the entire normalized level shape (compartments + stacks + layers + fasteners + tutorialSteps), duplicating the
kind: "layer"reference / extracted layer pattern thatcreateLevelindata.jsproduces. This makes the fixtures fragile to any future schema change. Reusing the same factory helpers (createLevel,compartment,pick,band,layer) — or a small test-onlybuildLevelwrapper — would keep these tests in sync with the production data shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/tests/engine.test.js` around lines 67 - 162, The test duplicates a full normalized level fixture instead of using existing builders, making it fragile; refactor this test to use the production/test factory helpers (e.g. createLevel, compartment, pick, band, layer or a small test-only buildLevel wrapper) to construct the same level shape, then call createRuntime, createInitialState and applyFastener as before; ensure you reproduce the same compartments, stacks/layers and fasteners (ids like "egg-box", "of-stack-a"/"of-stack-b", "of-a1"/"of-b1", "of-pick-a1"/"of-pick-b1") so assertions about second.failed, second.failKind and second.message remain valid.
311-313: ⚡ Quick winHardcoded layer/stack/compartment ranges are brittle.
The
>= 9 && <= 10,>= 4 && <= 5,>= 3 && <= 4bounds encode the currentDAILY_TEMPLATE_DEFINITIONSshape (template 1 has 4 stacks / 9 layers / 4 compartments; template 2 has 5 stacks / 11 layers / 4 compartments) plus an implicit assumption about which template the2026-05-05seed selects. Adding or tweaking a template can flip the test from passing to failing for unrelated reasons, with no actionable signal.The determinism check (
assert.deepEqual(first, second)),validateLevelround-trip, and ingredient-pool subset check already pin the contract that matters. Consider replacing the magic ranges with structural invariants (e.g., every layer has astackId, every layer'stargetCompartmentIdresolves to a compartment, layer count equals the sum of layer items instacks).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/tests/engine.test.js` around lines 311 - 313, Replace the brittle hardcoded range assertions on first.layers/first.stacks/first.compartments with structural invariants: assert every layer in first.layers has a non-null stackId and targetCompartmentId, assert each layer.targetCompartmentId resolves to an id present in first.compartments, assert every layer.stackId resolves to a stack in first.stacks, and assert first.layers.length equals the total number of layer references aggregated from first.stacks (e.g., sum of layers referenced by each stack). Keep the existing determinism check (assert.deepEqual(first, second)), validateLevel round-trip, and ingredient-pool subset checks; remove the specific numeric bounds that depend on DAILY_TEMPLATE_DEFINITIONS.bento-board/src/app.js (2)
23-34: 💤 Low value
appState.dailySeedbecomes stale across UTC midnight.
dailySeedis initialized once at module load and only refreshed insideloadDaily(...). If a user keeps the home screen open across UTC midnight (or the tab is backgrounded), the daily card on the home view (Line 648) and the unlocked-pool blurb (Line 658) keep showing yesterday's seed until they navigate. A simple recompute onopenHome()keeps the home card honest.♻️ Proposed refresh
function openHome() { appState.view = "home"; appState.modal = null; + appState.dailySeed = dailySeedFromDate(); refreshBoard(); updateTitle(); render(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/app.js` around lines 23 - 34, appState.dailySeed is computed once at module load (via dailySeedFromDate()) and can become stale across UTC midnight; update appState.dailySeed inside the openHome() function by calling dailySeedFromDate() and assigning the result to appState.dailySeed before any rendering or calls to loadDaily(...) so the home card and unlocked-pool blurb reflect the current seed; ensure any subsequent logic that depends on the seed (e.g., loadDaily or rendering code that reads appState.dailySeed) uses the refreshed value.
745-751: 💤 Low valueDead branch in
continueLatest.
isDailyUnlocked()returns true only whenhighestClearedLevelNumber() >= 6, which requires at least one entry inappState.progress.clearedLevels. SoisDailyUnlocked() && !appState.progress.clearedLevels.lengthcan never both be true, andloadLevelByIndex(0, ...)on Line 747 is unreachable. Either drop the branch or rewrite the intended fallback for "no clears yet" (which, on its own, is thelatestUnlockedLevelIndex() === 0case already handled by Line 750).♻️ Proposed simplification
function continueLatest() { - if (isDailyUnlocked() && !appState.progress.clearedLevels.length) { - loadLevelByIndex(0, { showStart: true }); - return; - } loadLevelByIndex(latestUnlockedLevelIndex(), { showStart: true }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/app.js` around lines 745 - 751, The conditional branch in continueLatest is dead because isDailyUnlocked() implies appState.progress.clearedLevels is non-empty, so the if (isDailyUnlocked() && !appState.progress.clearedLevels.length) path that calls loadLevelByIndex(0, { showStart: true }) is unreachable; remove that branch and simplify continueLatest to always call loadLevelByIndex(latestUnlockedLevelIndex(), { showStart: true }) (or, if the original intent was to fall back to index 0 when there are no clears, replace the condition with a check on appState.progress.clearedLevels.length instead of isDailyUnlocked())..github/workflows/bento-board-preview.yml (2)
141-168: ⚡ Quick winConsider disabling
cancel-in-progressfor stable deploys.The
concurrencyblock at the top of the file applies to every job, includingdeploy-stable. Withcancel-in-progress: true, two rapid pushes tomainwill cancel an in-flightpeaceiris/actions-gh-pagesstep, and an interrupted publish can leavegh-pageswithout the latest stable bento-board build until the next push triggers another run. Splitting concurrency so that PR previews cancel each other but main publishes queue would be safer.♻️ Proposed split
concurrency: group: bento-board-preview-${{ github.event.pull_request.number || github.ref_name }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/bento-board-preview.yml around lines 141 - 168, The top-level concurrency currently cancels in-progress runs and affects the deploy-stable job; update the workflow so deploy-stable does not use cancel-in-progress: true by adding a job-level concurrency block (or a separate concurrency group) for the deploy-stable job named explicitly (reference job name deploy-stable) and set cancel-in-progress: false (or use a distinct group for PR previews and a separate group for stable publishes) so that peaceiris/actions-gh-pages publishes are queued rather than cancelled.
58-63: 💤 Low valueRedundant step-level
envmapping.
PREVIEW_DIRandPREVIEW_URLare already defined at the job level (Lines 32-34) and are automatically inherited by every step'sprocess.env. Re-declaring them at the step level by reading fromenv.*is a no-op and adds noise.♻️ Proposed cleanup
- name: Comment preview URL on pull request uses: actions/github-script@v7 - env: - PREVIEW_DIR: ${{ env.PREVIEW_DIR }} - PREVIEW_URL: ${{ env.PREVIEW_URL }} with: script: |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/bento-board-preview.yml around lines 58 - 63, The step named "Comment preview URL on pull request" currently redeclares PREVIEW_DIR and PREVIEW_URL in a step-level env block even though those variables are already defined at the job level; remove the env: PREVIEW_DIR and env: PREVIEW_URL mapping from that step (the step using actions/github-script@v7) so it inherits the job-level environment variables and eliminates the redundant no-op mappings.bento-board/package.json (1)
1-11: 💤 Low valueConsider adding an
enginesfield.The CI workflow pins Node 22, but contributors running
npm run verifylocally have no signal about the required Node version. Adding anenginesfield documents the runtime requirement and lets npm warn on mismatches.♻️ Proposed addition
"type": "module", + "engines": { + "node": ">=22" + }, "scripts": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/package.json` around lines 1 - 11, Add an "engines" field to package.json to document the required Node runtime (CI pins Node 22); update the package.json root object to include something like "engines": { "node": "22.x" } so npm warns on mismatched local runtimes when developers run scripts such as the "verify" script; ensure the field is placed alongside existing top-level keys (name, version, type, scripts).bento-board/src/engine.js (4)
405-432: 💤 Low valueDefensive:
ingredient.familymay NPE on the overflow path.If a level slips through
validateLevelwith an unknowningredientTypeId(for example via the"A"/"B"/"C"/"D"carve-out flagged separately),INGREDIENT_TYPES[layer.ingredientTypeId]returnsundefinedand Line 423/431 throwTypeError: Cannot read properties of undefined (reading 'family')instead of producing a proper failure feedback. Falling back tolayer.ingredientTypeId(or a placeholder) keeps the failure path narratable.🛡️ Suggested fallback
- const ingredient = INGREDIENT_TYPES[layer.ingredientTypeId]; + const ingredient = INGREDIENT_TYPES[layer.ingredientTypeId] ?? { family: layer.ingredientTypeId }; const compartment = runtime.compartmentMap.get(resolved.compartmentId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/engine.js` around lines 405 - 432, The overflow failure path assumes INGREDIENT_TYPES[layer.ingredientTypeId] exists and uses ingredient.family, which can throw if ingredient is undefined; change the construction of the failure/feedback messages in the block that builds the overflow response (the returned object for simulateChain failure) to defensively resolve the ingredient display name by checking INGREDIENT_TYPES[layer.ingredientTypeId] and falling back to layer.ingredientTypeId or a placeholder like "unknown ingredient" before using it in message, hintAction, feedback.message, and message fields so the failure path never NPEs.
540-576: 💤 Low value
findHintre-runsapplyFasteneraftersolveLevelalready simulated the chain.
solveLevelcomputes the full sequence by callingapplyFasteneron every chosen fastener; the resulting chain length is already represented by the actions taken before the nextapplyFastener. CallingapplyFastener(level, state, action.id, runtime)again on Line 566 just to readchainLayerIds.lengthdoubles the work for hint requests (extra clones and array allocations) and adds anotherfeedbackSeqincrement that you immediately discard. Returning thechainLayerIdsfromsolveLevel's sequence (or storing it on the action object) avoids the second simulation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/engine.js` around lines 540 - 576, findHint currently re-simulates the chosen action by calling applyFastener to read chainLayerIds, doubling work; instead have solveLevel populate the resolution metadata on each action in solution.sequence (e.g. attach chainLayerIds or a chainCount property to the action object when solveLevel calls applyFastener) and return that in the solution; then update findHint to read the chain length from action.chainLayerIds.length or action.chainCount and remove the applyFastener call and preview/lastResolution usage so no extra cloning/feedbackSeq increment occurs.
496-538: ⚖️ Poor tradeoff
solveLevelrecursion + accumulated history can blow memory on harder authored levels.Each recursive
applyFastenerreturns a state whosehistorygrows by one snapshot, so the search frontier holds states with O(depth)-sized history arrays, andJSON.parse(JSON.stringify(...))clones them on every transition. For the current daily/campaign sizes this is fine, but if level authoring expands toward deeper stacks the search will both deepen and inflate per-state memory. An iterative search with explicit move/undo (or strippinghistoryfrom search-only states) would scale more predictably.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/engine.js` around lines 496 - 538, solveLevel's recursive search builds full game states (via applyFastener) that include ever-growing history arrays and are deep-cloned, causing memory blowup on deep searches; to fix, avoid storing or cloning history in states used only for search: modify search to either (a) strip or replace the history field with a lightweight marker before adding to the seen set / recursing, or (b) switch to an iterative DFS using applyFastener + a corresponding undo operation so you reuse a single mutable state buffer. Specifically, update search inside solveLevel to not carry history (remove/replace state.history) when calling stateSignature, seen.add, collectReadyFasteners, and applyFastener (or implement/invoke an undoFastener to revert applyFastener in the iterative stack), keeping function names solveLevel, search, applyFastener, collectReadyFasteners, stateSignature and history as the touch points.
715-721: ⚡ Quick winThe
["A","B","C","D"]allowlist in validation is overly permissive and masks potential defects.Tokens "A" through "D" are template placeholders replaced by
instantiateTemplatebefore any level reaches validation. All code paths callvalidateLevelpost-instantiation (daily.js line 127) or on handcrafted levels with real ingredient IDs. However, the explicit allowlist at line 718 permits tokens to pass validation even though they should never appear in validated levels. If code refactors in the future, this silent acceptance could hide a real defect—a template with unsubstituted tokens passing validation then crashing downstream when consumers referenceINGREDIENT_TYPES[layer.ingredientTypeId](e.g., line 408). Removing the allowlist or documenting it as test-only would keep validation strict for production levels.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/engine.js` around lines 715 - 721, The validation currently permits template tokens by checking ["A","B","C","D"] when validating layer.ingredientTypeId, which masks unsubstituted templates; update the validation in the function that uses runtime.compartmentMap and INGREDIENT_TYPES so it no longer allowlists those tokens—remove the || ["A","B","C","D"].includes(layer.ingredientTypeId) branch and only accept IDs present in INGREDIENT_TYPES, and if you need to preserve convenience for tests, gate any token allowlist behind an explicit test-only flag or environment check used by instantiateTemplate/test harness so validateLevel always enforces production strictness.bento-board/src/daily.js (2)
109-112: 💤 Low valueConfirm intent of the
getUnlockedDailyPool(6)default.
getUnlockedDailyPoolonly changes its return athighestClearedLevelNumber >= 12, so passing6is functionally identical to passing0. If the intent is "default to the starter pool",0is more honest; if it's meant to track a specific unlock threshold, the numeric literal would benefit from a named constant or comment. As written, the magic6reads like an unlock checkpoint that does not actually exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/daily.js` around lines 109 - 112, The default argument for unlockedIngredientIds in generateDailyLevel currently calls getUnlockedDailyPool(6), which is misleading because getUnlockedDailyPool only changes behavior at highestClearedLevelNumber >= 12; change the default to call getUnlockedDailyPool(0) if the intent is to use the starter pool, or replace the numeric literal with a descriptive constant (e.g., DEFAULT_STARTER_POOL_THRESHOLD or STARTER_POOL_LEVEL) and use that constant in the call to getUnlockedDailyPool to make intent explicit; update any related references in generateDailyLevel and document the choice in a short comment near dailySeedFromDate()/getUnlockedDailyPool for clarity.
109-138: ⚡ Quick winPool sufficiency check is decoupled from template requirements.
The hard-coded
pool.length < 4guard on Line 118 only ensures a global minimum. If anyDAILY_TEMPLATE_DEFINITIONSentry hastokenOrder.length > pool.length,shuffle(pool, attemptRandom).slice(0, template.tokenOrder.length)silently returns an array shorter thantokenOrder, leaving trailing entries intokenMapasundefined.instantiateTemplatethen produces compartments labeled"undefined Bay"and layers withingredientTypeId: undefined, which only fail later viavalidateLevel. After exhaustingtemplateOrder.length * 6attempts, the caller gets the generic"Unable to generate a valid daily special for ${seed}.", masking the real root cause.Consider validating the pool against the actual template requirements and/or skipping templates that exceed the pool early so failures surface clearly.
♻️ Suggested change
const baseSeed = hashSeed(seed); const random = mulberry32(baseSeed); const templateOrder = shuffle(DAILY_TEMPLATE_DEFINITIONS, random); const pool = unlockedIngredientIds.filter((ingredientId) => Boolean(INGREDIENT_TYPES[ingredientId])); - if (pool.length < 4) { - throw new Error("Daily generation requires at least four unlocked ingredients."); + const requiredTokens = Math.max( + ...DAILY_TEMPLATE_DEFINITIONS.map((template) => template.tokenOrder.length) + ); + if (pool.length < requiredTokens) { + throw new Error( + `Daily generation requires at least ${requiredTokens} unlocked ingredients (have ${pool.length}).` + ); } for (let attempt = 0; attempt < templateOrder.length * 6; attempt += 1) { const template = templateOrder[attempt % templateOrder.length]; + if (pool.length < template.tokenOrder.length) { + continue; + } const attemptRandom = mulberry32(baseSeed + attempt * 97);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bento-board/src/daily.js` around lines 109 - 138, The pool-length guard in generateDailyLevel is insufficient because some DAILY_TEMPLATE_DEFINITIONS have tokenOrder.length larger than pool, causing undefined ingredients downstream; fix by comparing each template.tokenOrder.length against the computed pool.length (or compute the overall max required from DAILY_TEMPLATE_DEFINITIONS) and skip any template whose tokenOrder.length > pool.length before attempting to shuffle/instantiate (or throw a clear error mentioning insufficient unlocked ingredients), so that instantiateTemplate and validateLevel never receive undefined ingredient ids; update generateDailyLevel to perform this per-template check (referencing template.tokenOrder.length, pool, DAILY_TEMPLATE_DEFINITIONS, instantiateTemplate, and validateLevel) and return/continue with a descriptive error or skip instead of letting validation fail later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/bento-board-preview.yml:
- Around line 76-100: The marker-search currently only inspects the first page
because github.rest.issues.listComments(...) defaults to 30 items, so update the
logic that finds `existing` to fetch all comments (e.g., use `github.paginate`
or call `github.rest.issues.listComments` with `per_page: 100` and paginate) and
then search for the comment whose `user.type === "Bot"` and
`body.includes(marker)` before deciding to call
`github.rest.issues.updateComment` or `github.rest.issues.createComment`; ensure
you replace the single-page `comments` result used when constructing `existing`
so duplicate preview comments are not created on long-lived PRs.
In `@bento-board/README.md`:
- Around line 5-7: The README's "Open `index.html` in a browser" suggestion is
incorrect because bento-board/src/app.js uses ES module imports (import ... from
"./daily.js") loaded via <script type="module"> which will usually fail over
file://; update the Run section to remove or de-emphasize the file:// option and
lead with instructions to serve the directory with a static file server (or only
describe using a static server), and mention that ES modules require HTTP(S) so
browsers will block module loading via file:// when loading index.html that
references app.js.
In `@bento-board/src/app.js`:
- Around line 53-55: The saveProgress function can throw from
localStorage.setItem and break the win flow (called by recordWin/applyPuzzle);
wrap the JSON.stringify + localStorage.setItem call in a try/catch (mirroring
loadProgress) so any QuotaExceededError or SecurityError is caught, log the
error (or silently ignore) and do not rethrow so the appState changes (e.g.,
appState.modal = "win") and render still run; update saveProgress (and reference
STORAGE_KEY and appState.progress) to swallow storage errors safely.
In `@bento-board/styles.css`:
- Around line 673-701: The pulse-ring animation used by .is-highlighted applies
translateX(-50%) which shifts elements without a centered base transform
(breaking .fastener--band); modify the styling so only centered fasteners use
pulse-ring (e.g., keep .fastener--pick and .fastener--divider using
.is-highlighted with pulse-ring) and introduce a new transform-free keyframe
(e.g., pulse-scale) and apply it to .fastener--band.is-highlighted (or scope
.is-highlighted to centered fasteners only) so bands are pulsed by scale only
and not translated.
---
Nitpick comments:
In @.github/workflows/bento-board-preview.yml:
- Around line 141-168: The top-level concurrency currently cancels in-progress
runs and affects the deploy-stable job; update the workflow so deploy-stable
does not use cancel-in-progress: true by adding a job-level concurrency block
(or a separate concurrency group) for the deploy-stable job named explicitly
(reference job name deploy-stable) and set cancel-in-progress: false (or use a
distinct group for PR previews and a separate group for stable publishes) so
that peaceiris/actions-gh-pages publishes are queued rather than cancelled.
- Around line 58-63: The step named "Comment preview URL on pull request"
currently redeclares PREVIEW_DIR and PREVIEW_URL in a step-level env block even
though those variables are already defined at the job level; remove the env:
PREVIEW_DIR and env: PREVIEW_URL mapping from that step (the step using
actions/github-script@v7) so it inherits the job-level environment variables and
eliminates the redundant no-op mappings.
In `@bento-board/package.json`:
- Around line 1-11: Add an "engines" field to package.json to document the
required Node runtime (CI pins Node 22); update the package.json root object to
include something like "engines": { "node": "22.x" } so npm warns on mismatched
local runtimes when developers run scripts such as the "verify" script; ensure
the field is placed alongside existing top-level keys (name, version, type,
scripts).
In `@bento-board/src/app.js`:
- Around line 23-34: appState.dailySeed is computed once at module load (via
dailySeedFromDate()) and can become stale across UTC midnight; update
appState.dailySeed inside the openHome() function by calling dailySeedFromDate()
and assigning the result to appState.dailySeed before any rendering or calls to
loadDaily(...) so the home card and unlocked-pool blurb reflect the current
seed; ensure any subsequent logic that depends on the seed (e.g., loadDaily or
rendering code that reads appState.dailySeed) uses the refreshed value.
- Around line 745-751: The conditional branch in continueLatest is dead because
isDailyUnlocked() implies appState.progress.clearedLevels is non-empty, so the
if (isDailyUnlocked() && !appState.progress.clearedLevels.length) path that
calls loadLevelByIndex(0, { showStart: true }) is unreachable; remove that
branch and simplify continueLatest to always call
loadLevelByIndex(latestUnlockedLevelIndex(), { showStart: true }) (or, if the
original intent was to fall back to index 0 when there are no clears, replace
the condition with a check on appState.progress.clearedLevels.length instead of
isDailyUnlocked()).
In `@bento-board/src/daily.js`:
- Around line 109-112: The default argument for unlockedIngredientIds in
generateDailyLevel currently calls getUnlockedDailyPool(6), which is misleading
because getUnlockedDailyPool only changes behavior at highestClearedLevelNumber
>= 12; change the default to call getUnlockedDailyPool(0) if the intent is to
use the starter pool, or replace the numeric literal with a descriptive constant
(e.g., DEFAULT_STARTER_POOL_THRESHOLD or STARTER_POOL_LEVEL) and use that
constant in the call to getUnlockedDailyPool to make intent explicit; update any
related references in generateDailyLevel and document the choice in a short
comment near dailySeedFromDate()/getUnlockedDailyPool for clarity.
- Around line 109-138: The pool-length guard in generateDailyLevel is
insufficient because some DAILY_TEMPLATE_DEFINITIONS have tokenOrder.length
larger than pool, causing undefined ingredients downstream; fix by comparing
each template.tokenOrder.length against the computed pool.length (or compute the
overall max required from DAILY_TEMPLATE_DEFINITIONS) and skip any template
whose tokenOrder.length > pool.length before attempting to shuffle/instantiate
(or throw a clear error mentioning insufficient unlocked ingredients), so that
instantiateTemplate and validateLevel never receive undefined ingredient ids;
update generateDailyLevel to perform this per-template check (referencing
template.tokenOrder.length, pool, DAILY_TEMPLATE_DEFINITIONS,
instantiateTemplate, and validateLevel) and return/continue with a descriptive
error or skip instead of letting validation fail later.
In `@bento-board/src/data.js`:
- Around line 80-165: Extract the duplicated stack-normalization logic into a
single helper (e.g., normalizeStacksAndExtractLayers or extractLayersFromStacks)
that takes the stacks array and returns { stacks: normalizedStacks, layers }
where each extracted layer is annotated with stackId and each original layer
item is replaced with { kind: "layer", id }. Replace the inlined mapping in
createLevel and createDailyTemplate to call this helper and assign returned
stacks to the builders' stacks field and returned layers to their layers
variable, preserving all other fields and the default tutorialSteps behavior in
createLevel.
In `@bento-board/src/engine.js`:
- Around line 405-432: The overflow failure path assumes
INGREDIENT_TYPES[layer.ingredientTypeId] exists and uses ingredient.family,
which can throw if ingredient is undefined; change the construction of the
failure/feedback messages in the block that builds the overflow response (the
returned object for simulateChain failure) to defensively resolve the ingredient
display name by checking INGREDIENT_TYPES[layer.ingredientTypeId] and falling
back to layer.ingredientTypeId or a placeholder like "unknown ingredient" before
using it in message, hintAction, feedback.message, and message fields so the
failure path never NPEs.
- Around line 540-576: findHint currently re-simulates the chosen action by
calling applyFastener to read chainLayerIds, doubling work; instead have
solveLevel populate the resolution metadata on each action in solution.sequence
(e.g. attach chainLayerIds or a chainCount property to the action object when
solveLevel calls applyFastener) and return that in the solution; then update
findHint to read the chain length from action.chainLayerIds.length or
action.chainCount and remove the applyFastener call and preview/lastResolution
usage so no extra cloning/feedbackSeq increment occurs.
- Around line 496-538: solveLevel's recursive search builds full game states
(via applyFastener) that include ever-growing history arrays and are
deep-cloned, causing memory blowup on deep searches; to fix, avoid storing or
cloning history in states used only for search: modify search to either (a)
strip or replace the history field with a lightweight marker before adding to
the seen set / recursing, or (b) switch to an iterative DFS using applyFastener
+ a corresponding undo operation so you reuse a single mutable state buffer.
Specifically, update search inside solveLevel to not carry history
(remove/replace state.history) when calling stateSignature, seen.add,
collectReadyFasteners, and applyFastener (or implement/invoke an undoFastener to
revert applyFastener in the iterative stack), keeping function names solveLevel,
search, applyFastener, collectReadyFasteners, stateSignature and history as the
touch points.
- Around line 715-721: The validation currently permits template tokens by
checking ["A","B","C","D"] when validating layer.ingredientTypeId, which masks
unsubstituted templates; update the validation in the function that uses
runtime.compartmentMap and INGREDIENT_TYPES so it no longer allowlists those
tokens—remove the || ["A","B","C","D"].includes(layer.ingredientTypeId) branch
and only accept IDs present in INGREDIENT_TYPES, and if you need to preserve
convenience for tests, gate any token allowlist behind an explicit test-only
flag or environment check used by instantiateTemplate/test harness so
validateLevel always enforces production strictness.
In `@bento-board/tests/engine.test.js`:
- Around line 67-162: The test duplicates a full normalized level fixture
instead of using existing builders, making it fragile; refactor this test to use
the production/test factory helpers (e.g. createLevel, compartment, pick, band,
layer or a small test-only buildLevel wrapper) to construct the same level
shape, then call createRuntime, createInitialState and applyFastener as before;
ensure you reproduce the same compartments, stacks/layers and fasteners (ids
like "egg-box", "of-stack-a"/"of-stack-b", "of-a1"/"of-b1",
"of-pick-a1"/"of-pick-b1") so assertions about second.failed, second.failKind
and second.message remain valid.
- Around line 311-313: Replace the brittle hardcoded range assertions on
first.layers/first.stacks/first.compartments with structural invariants: assert
every layer in first.layers has a non-null stackId and targetCompartmentId,
assert each layer.targetCompartmentId resolves to an id present in
first.compartments, assert every layer.stackId resolves to a stack in
first.stacks, and assert first.layers.length equals the total number of layer
references aggregated from first.stacks (e.g., sum of layers referenced by each
stack). Keep the existing determinism check (assert.deepEqual(first, second)),
validateLevel round-trip, and ingredient-pool subset checks; remove the specific
numeric bounds that depend on DAILY_TEMPLATE_DEFINITIONS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de934b84-547d-4367-ba86-1f8f1b77fe29
📒 Files selected for processing (10)
.github/workflows/bento-board-preview.ymlbento-board/README.mdbento-board/index.htmlbento-board/package.jsonbento-board/src/app.jsbento-board/src/daily.jsbento-board/src/data.jsbento-board/src/engine.jsbento-board/styles.cssbento-board/tests/engine.test.js
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number | ||
| }); | ||
|
|
||
| const existing = comments.find( | ||
| (comment) => comment.user.type === "Bot" && comment.body.includes(marker) | ||
| ); | ||
|
|
||
| if (existing) { | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: existing.id, | ||
| body | ||
| }); | ||
| } else { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body | ||
| }); | ||
| } |
There was a problem hiding this comment.
Marker comment can be missed on long-lived PRs.
github.rest.issues.listComments({...}) returns only the first 30 comments by default. On a long-running PR (review threads, CI bots, etc.), the marker comment may sit beyond page 1 and existing will be undefined, causing a duplicate preview comment to be posted on every redeploy. Use paginate (or a higher per_page) so the marker is reliably found.
🐛 Proposed fix
- const { data: comments } = await github.rest.issues.listComments({
+ const comments = await github.paginate(github.rest.issues.listComments, {
owner: context.repo.owner,
repo: context.repo.repo,
- issue_number: context.issue.number
+ issue_number: context.issue.number,
+ per_page: 100
});
const existing = comments.find(
- (comment) => comment.user.type === "Bot" && comment.body.includes(marker)
+ (comment) => comment.user?.type === "Bot" && comment.body?.includes(marker)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: comments } = await github.rest.issues.listComments({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: context.issue.number | |
| }); | |
| const existing = comments.find( | |
| (comment) => comment.user.type === "Bot" && comment.body.includes(marker) | |
| ); | |
| if (existing) { | |
| await github.rest.issues.updateComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| comment_id: existing.id, | |
| body | |
| }); | |
| } else { | |
| await github.rest.issues.createComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: context.issue.number, | |
| body | |
| }); | |
| } | |
| const comments = await github.paginate(github.rest.issues.listComments, { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: context.issue.number, | |
| per_page: 100 | |
| }); | |
| const existing = comments.find( | |
| (comment) => comment.user?.type === "Bot" && comment.body?.includes(marker) | |
| ); | |
| if (existing) { | |
| await github.rest.issues.updateComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| comment_id: existing.id, | |
| body | |
| }); | |
| } else { | |
| await github.rest.issues.createComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: context.issue.number, | |
| body | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/bento-board-preview.yml around lines 76 - 100, The
marker-search currently only inspects the first page because
github.rest.issues.listComments(...) defaults to 30 items, so update the logic
that finds `existing` to fetch all comments (e.g., use `github.paginate` or call
`github.rest.issues.listComments` with `per_page: 100` and paginate) and then
search for the comment whose `user.type === "Bot"` and `body.includes(marker)`
before deciding to call `github.rest.issues.updateComment` or
`github.rest.issues.createComment`; ensure you replace the single-page
`comments` result used when constructing `existing` so duplicate preview
comments are not created on long-lived PRs.
| ## Run | ||
|
|
||
| Open `index.html` in a browser, or serve the directory with any static file server. |
There was a problem hiding this comment.
file:// will not load ES modules in most browsers.
bento-board/src/app.js uses ES module imports (import { ... } from "./daily.js") and is loaded via <script type="module">. Most browsers refuse to load ES modules over file:// due to CORS, so "Open index.html in a browser" will fail with a console error for the typical case. Recommend leading with the static-server option (or removing the file:// suggestion altogether).
📝 Proposed wording fix
-Open `index.html` in a browser, or serve the directory with any static file server.
+Serve the directory with any static file server (for example `npx http-server bento-board` or `python3 -m http.server -d bento-board`). Opening `index.html` directly via `file://` will not work because `src/app.js` is loaded as an ES module.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Run | |
| Open `index.html` in a browser, or serve the directory with any static file server. | |
| ## Run | |
| Serve the directory with any static file server (for example `npx http-server bento-board` or `python3 -m http.server -d bento-board`). Opening `index.html` directly via `file://` will not work because `src/app.js` is loaded as an ES module. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bento-board/README.md` around lines 5 - 7, The README's "Open `index.html` in
a browser" suggestion is incorrect because bento-board/src/app.js uses ES module
imports (import ... from "./daily.js") loaded via <script type="module"> which
will usually fail over file://; update the Run section to remove or de-emphasize
the file:// option and lead with instructions to serve the directory with a
static file server (or only describe using a static server), and mention that ES
modules require HTTP(S) so browsers will block module loading via file:// when
loading index.html that references app.js.
| function saveProgress() { | ||
| localStorage.setItem(STORAGE_KEY, JSON.stringify(appState.progress)); | ||
| } |
There was a problem hiding this comment.
saveProgress can throw and break the win flow.
localStorage.setItem raises a QuotaExceededError when storage is full, and a SecurityError in private/incognito modes or when third-party storage is blocked. saveProgress is called from recordWin (Line 154), which is called from applyPuzzle (Line 193) immediately before appState.modal = "win" and render() are scheduled — so a throw here aborts the win flow, leaving the puzzle in completed: true but the win modal never shown.
loadProgress already wraps localStorage in a try/catch (Lines 37-50); save should match.
🛡️ Proposed guard
function saveProgress() {
- localStorage.setItem(STORAGE_KEY, JSON.stringify(appState.progress));
+ try {
+ localStorage.setItem(STORAGE_KEY, JSON.stringify(appState.progress));
+ } catch {
+ // Quota / private-mode / blocked-storage failures should not break gameplay.
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function saveProgress() { | |
| localStorage.setItem(STORAGE_KEY, JSON.stringify(appState.progress)); | |
| } | |
| function saveProgress() { | |
| try { | |
| localStorage.setItem(STORAGE_KEY, JSON.stringify(appState.progress)); | |
| } catch { | |
| // Quota / private-mode / blocked-storage failures should not break gameplay. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bento-board/src/app.js` around lines 53 - 55, The saveProgress function can
throw from localStorage.setItem and break the win flow (called by
recordWin/applyPuzzle); wrap the JSON.stringify + localStorage.setItem call in a
try/catch (mirroring loadProgress) so any QuotaExceededError or SecurityError is
caught, log the error (or silently ignore) and do not rethrow so the appState
changes (e.g., appState.modal = "win") and render still run; update saveProgress
(and reference STORAGE_KEY and appState.progress) to swallow storage errors
safely.
| .is-highlighted { | ||
| animation: pulse-ring 0.9s ease-in-out infinite alternate; | ||
| } | ||
|
|
||
| .ingredient-layer.is-highlighted, | ||
| .recipe-card.is-highlighted, | ||
| .compartment.is-highlighted { | ||
| animation: tray-pulse 0.55s ease; | ||
| } | ||
|
|
||
| @keyframes lift-in { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateY(10px); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| } | ||
|
|
||
| @keyframes pulse-ring { | ||
| from { | ||
| transform: translateX(-50%) scale(1); | ||
| } | ||
| to { | ||
| transform: translateX(-50%) scale(1.04); | ||
| } | ||
| } |
There was a problem hiding this comment.
pulse-ring shifts band fasteners off-center when highlighted.
.is-highlighted (Line 673) applies the pulse-ring keyframe, which forces transform: translateX(-50%) scale(...) on the element. This is correct for .fastener--pick and .fastener--divider (both use translateX(-50%) as their base transform), but .fastener--band is laid out with left: 10px; right: 10px; and no base transform. When a band is highlighted (e.g., during the picnic-02 / picnic-05 tutorials that highlight l2-band-a / l5-band-a), the animation translates the band -50% horizontally and visually breaks the layout.
Either limit pulse-ring to centered fasteners, or use a transform-free pulse for bands.
♻️ Proposed scoping
-.is-highlighted {
+.fastener--pick.is-highlighted,
+.fastener--divider.is-highlighted,
+.divider-tab.is-highlighted,
+.stack-card__count.is-highlighted {
animation: pulse-ring 0.9s ease-in-out infinite alternate;
}
+
+.fastener--band.is-highlighted .fastener__shape {
+ animation: tray-pulse 0.9s ease-in-out infinite alternate;
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bento-board/styles.css` around lines 673 - 701, The pulse-ring animation used
by .is-highlighted applies translateX(-50%) which shifts elements without a
centered base transform (breaking .fastener--band); modify the styling so only
centered fasteners use pulse-ring (e.g., keep .fastener--pick and
.fastener--divider using .is-highlighted with pulse-ring) and introduce a new
transform-free keyframe (e.g., pulse-scale) and apply it to
.fastener--band.is-highlighted (or scope .is-highlighted to centered fasteners
only) so bands are pulsed by scale only and not translated.
1 participant
Summary
bento-boardValidation
npm run verify(bento-board)Workflow diagnosis
GAM-19finished with an emptypr_url, and the originalagent/gameplay-engineer/0ccde624worktree still hadbento-board/as untracked files, so there was no reproducible branch tip to open a PR from.Summary by CodeRabbit
New Features
Documentation
Tests
Chores